-
Notifications
You must be signed in to change notification settings - Fork 452
Bump Go toolchain to v1.24.6 #5813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold because we may want to do the v1.21 release first |
@mboersma if this passes tests I'd be keen to include in 1.21 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5813 +/- ##
==========================================
- Coverage 53.06% 46.92% -6.15%
==========================================
Files 279 279
Lines 29677 29677
==========================================
- Hits 15749 13926 -1823
- Misses 13112 14938 +1826
+ Partials 816 813 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/retest |
1 similar comment
/retest |
@mboersma: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Could we also bump the version here? diff --git a/cloudbuild.yaml b/cloudbuild.yaml
index 79dcdcec3..e584386e8 100644
--- a/cloudbuild.yaml
+++ b/cloudbuild.yaml
@@ -4,7 +4,7 @@ options:
substitution_option: ALLOW_LOOSE
machineType: 'E2_HIGHCPU_8'
steps:
- - name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250116-2a05ea7e3d' # Go 1.23
+ - name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079' # Go 1.24
entrypoint: make
env:
- DOCKER_CLI_EXPERIMENTAL=enabled curl -sL gcr.io/v2/k8s-staging-test-infra/gcb-docker-gcloud/tags/list | jq '.tags' % docker run --rm --entrypoint go gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079 version
go version go1.24.3 linux/amd64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These couple comments are totally non-blocking. Would be nice to update the cloudbuild.yaml as part of this PR though.
And for next time I think it would help reviewing to split this into separate commits for a) bumping the Go version, and b) each logic group of linter issues fixed.
cacheExpect: func(cache *mock_azure.MockCredentialCache) { | ||
ctx := context.Background() | ||
ctx := context.Background() //nolint:usetesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we refactor this function to take a context or *testing.T
argument so we can satisfy the linter. Otherwise, moving this ctx
to the top of TestGetTokenCredential
also appeases the linter, in case either of those are worth doing now.
diff --git a/azure/scope/identity_test.go b/azure/scope/identity_test.go
index 669fe204e..bcaa4aabd 100644
--- a/azure/scope/identity_test.go
+++ b/azure/scope/identity_test.go
@@ -17,7 +17,6 @@ limitations under the License.
package scope
import (
- "context"
"os"
"reflect"
"testing"
@@ -208,6 +207,7 @@ func TestHasClientSecret(t *testing.T) {
}
func TestGetTokenCredential(t *testing.T) {
+ ctx := t.Context()
testCertPath := "../../test/setup/certificate"
tests := []struct {
@@ -439,7 +439,6 @@ func TestGetTokenCredential(t *testing.T) {
},
},
cacheExpect: func(cache *mock_azure.MockCredentialCache) {
- ctx := context.Background() //nolint:usetesting
credsPath := "../../test/setup/credentials.json" //nolint:gosec
clientOptions := azcore.ClientOptions{
Cloud: cloud.Configuration{
mapper, err := AzureManagedClusterToAzureManagedMachinePoolsMapper(t.Context(), fakeClient, scheme, logr.New(sink)) | ||
g.Expect(err).NotTo(HaveOccurred()) | ||
|
||
requests := mapper(context.TODO(), &infrav1.AzureManagedCluster{ | ||
requests := mapper(t.Context(), &infrav1.AzureManagedCluster{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to eventually refactor the spots like this to define a root ctx
in one place and pass that around instead of each reinstantiating a separate t.Context()
.
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Updates the Go compiler and tools to v1.24.6.
I separated this out from #5661 because it's XL-sized on its own.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
TODOs:
Release note: